Refactor CustomQueryHotkeySetting control #3803
Conversation
|
🥷 Code experts: onesounds, Yusyuriv Jack251970, onesounds have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the CustomQueryHotkeySetting control to match the styling and behavior of CustomShortcutSetting, centralizes validation and dialog flow, and updates the underlying model and localization.
- Reworked SettingsPaneHotkeyViewModel to use new constructors, handle dialog results, and update hotkeys via HotKeyMapper
- Refactored CustomQueryHotkeySetting window class: unified add/update constructors, replaced inline update logic, and exposed Hotkey/ActionKeyword as bindable properties
- Added equality overrides in CustomPluginHotkey model and extended localization keys for empty/invalid states
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher/SettingPages/ViewModels/SettingsPaneHotkeyViewModel.cs | Swapped old dialog calls for new constructors and result handling |
| Flow.Launcher/Languages/en.xaml | Updated “invalidPluginHotkey”, added emptyPluginHotkey and invalidShortcut strings |
| Flow.Launcher/CustomQueryHotkeySetting.xaml.cs | Major overhaul of add/update logic, validation, and public properties |
| Flow.Launcher/CustomQueryHotkeySetting.xaml | Adjusted bindings and button visuals for add/update modes |
| Flow.Launcher.Infrastructure/UserSettings/PluginHotkey.cs | Renamed model to CustomPluginHotkey, added ctor, Equals/GetHashCode |
Comments suppressed due to low confidence (2)
Flow.Launcher.Infrastructure/UserSettings/PluginHotkey.cs:1
- The file name
PluginHotkey.csdoes not match the class nameCustomPluginHotkey. Consider renaming the file toCustomPluginHotkey.csfor consistency.
using System;
Flow.Launcher/CustomQueryHotkeySetting.xaml.cs:40
- [nitpick] Consider adding a check here to prevent assigning a hotkey that already exists in
Settings.CustomPluginHotkeys, similar to the duplicate shortcut check inCustomShortcutSetting.
Hotkey = HotkeyControl.CurrentHotkey.ToString();
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 Walkthrough""" WalkthroughThe changes refactor the management of custom plugin hotkeys and shortcuts by moving responsibility for collection updates and hotkey registration from the UI dialog to the view model. New validation logic ensures hotkeys and shortcuts exist before editing, and error messages are improved. UI elements and localization strings are updated to support these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPaneHotkeyViewModel
participant CustomQueryHotkeySetting
participant Settings
participant HotkeyMapper
User->>SettingsPaneHotkeyViewModel: Initiate Add/Edit Custom Hotkey
SettingsPaneHotkeyViewModel->>CustomQueryHotkeySetting: Open dialog (with or without existing hotkey)
CustomQueryHotkeySetting->>User: User enters hotkey and action keyword
User->>CustomQueryHotkeySetting: Confirm (OK)
CustomQueryHotkeySetting->>SettingsPaneHotkeyViewModel: Return Hotkey and ActionKeyword
SettingsPaneHotkeyViewModel->>Settings: Validate and update hotkey collection
SettingsPaneHotkeyViewModel->>HotkeyMapper: Register/Unregister hotkey as needed
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Flow.Launcher.Infrastructure/UserSettings/PluginHotkey.cs(1 hunks)Flow.Launcher/CustomQueryHotkeySetting.xaml(2 hunks)Flow.Launcher/CustomQueryHotkeySetting.xaml.cs(2 hunks)Flow.Launcher/CustomShortcutSetting.xaml.cs(1 hunks)Flow.Launcher/Languages/en.xaml(2 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneHotkeyViewModel.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
Flow.Launcher/CustomShortcutSetting.xaml.cs (4)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3672
File: Flow.Launcher/MainWindow.xaml.cs:244-247
Timestamp: 2025-06-08T14:12:21.348Z
Learning: In Flow.Launcher, App.NotifyIcon is created before MainWindow creation, so null checks for App.NotifyIcon are not necessary when accessing it from MainWindow code.
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#3394
File: Flow.Launcher/Themes/Darker Glass.xaml:134-141
Timestamp: 2025-03-28T21:12:13.386Z
Learning: In Flow.Launcher, hotkey styling is implemented with a two-component structure: a Border element with style `ItemHotkeyBGStyle` that provides background and border styling, containing a TextBlock with style `ItemHotkeyStyle` that handles the text styling.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3672
File: Flow.Launcher/MainWindow.xaml.cs:318-318
Timestamp: 2025-06-08T14:12:12.842Z
Learning: In Flow.Launcher, the App.NotifyIcon static property is initialized in the App class before MainWindow creation, so null checks are not needed when accessing App.NotifyIcon in MainWindow lifecycle methods.
Flow.Launcher/Languages/en.xaml (2)
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#3394
File: Flow.Launcher/Themes/Darker Glass.xaml:134-141
Timestamp: 2025-03-28T21:12:13.386Z
Learning: In Flow.Launcher, hotkey styling is implemented with a two-component structure: a Border element with style `ItemHotkeyBGStyle` that provides background and border styling, containing a TextBlock with style `ItemHotkeyStyle` that handles the text styling.
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Flow.Launcher.Infrastructure/UserSettings/PluginHotkey.cs (4)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#3394
File: Flow.Launcher/Themes/Darker Glass.xaml:134-141
Timestamp: 2025-03-28T21:12:13.386Z
Learning: In Flow.Launcher, hotkey styling is implemented with a two-component structure: a Border element with style `ItemHotkeyBGStyle` that provides background and border styling, containing a TextBlock with style `ItemHotkeyStyle` that handles the text styling.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-01-18T10:10:18.414Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Flow.Launcher/CustomQueryHotkeySetting.xaml (4)
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#3394
File: Flow.Launcher/Themes/Darker Glass.xaml:134-141
Timestamp: 2025-03-28T21:12:13.386Z
Learning: In Flow.Launcher, hotkey styling is implemented with a two-component structure: a Border element with style `ItemHotkeyBGStyle` that provides background and border styling, containing a TextBlock with style `ItemHotkeyStyle` that handles the text styling.
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#3394
File: Flow.Launcher/Themes/Darker Glass.xaml:134-141
Timestamp: 2025-03-28T21:20:54.978Z
Learning: In WPF applications like Flow.Launcher, Border elements cannot directly display text content and require a child element like TextBlock to handle text rendering. This separation of concerns (Border for visual container styling, TextBlock for text display) follows WPF best practices and provides greater styling flexibility.
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#3394
File: Flow.Launcher/Themes/Darker Glass.xaml:134-141
Timestamp: 2025-03-28T21:20:54.978Z
Learning: In WPF applications like Flow.Launcher, Border elements cannot directly display text content and require a child element like TextBlock to handle text rendering. This separation of concerns (Border for visual container styling, TextBlock for text display) follows WPF best practices and provides greater styling flexibility.
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#0
File: :0-0
Timestamp: 2025-04-23T15:14:49.986Z
Learning: In WPF applications like Flow.Launcher, font styling should be applied using implicit styles instead of setting the FontFamily property on individual controls. Define implicit styles in a ResourceDictionary using <Style TargetType="{x:Type Button}"> format and merge it into App.xaml, which automatically applies the font to all instances of the control type while still allowing explicit overrides where needed.
Flow.Launcher/SettingPages/ViewModels/SettingsPaneHotkeyViewModel.cs (2)
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#3394
File: Flow.Launcher/Themes/Darker Glass.xaml:134-141
Timestamp: 2025-03-28T21:12:13.386Z
Learning: In Flow.Launcher, hotkey styling is implemented with a two-component structure: a Border element with style `ItemHotkeyBGStyle` that provides background and border styling, containing a TextBlock with style `ItemHotkeyStyle` that handles the text styling.
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Flow.Launcher/CustomQueryHotkeySetting.xaml.cs (3)
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#3394
File: Flow.Launcher/Themes/Darker Glass.xaml:134-141
Timestamp: 2025-03-28T21:12:13.386Z
Learning: In Flow.Launcher, hotkey styling is implemented with a two-component structure: a Border element with style `ItemHotkeyBGStyle` that provides background and border styling, containing a TextBlock with style `ItemHotkeyStyle` that handles the text styling.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
🪛 GitHub Check: Check Spelling
Flow.Launcher/CustomQueryHotkeySetting.xaml
[warning] 162-162:
lbl is not a recognized word. (unrecognized-spelling)
Flow.Launcher/CustomQueryHotkeySetting.xaml.cs
[warning] 28-28:
lbl is not a recognized word. (unrecognized-spelling)
[warning] 19-19:
lbl is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (14)
Flow.Launcher/CustomShortcutSetting.xaml.cs (1)
46-46: LGTM! Code readability improved.The added blank lines around conditional validation blocks improve code readability and follow good formatting practices.
Also applies to: 53-53
Flow.Launcher/CustomQueryHotkeySetting.xaml (2)
123-123: LGTM! Data binding properly implemented.The two-way data binding for
ActionKeywordenables proper communication between the UI and the dialog's data properties.
154-167: LGTM! Dynamic button labels implemented correctly.The Grid structure with collapsed TextBlocks allows for runtime toggling between "done" and "update" labels, supporting both add and edit modes. This approach follows WPF best practices for conditional UI display.
Note: The static analysis warning about "lbl" is a false positive - "lbl" is a standard abbreviation for "label" in UI control naming conventions.
Flow.Launcher/Languages/en.xaml (1)
432-432: LGTM! Improved error messages for better user experience.The localization updates provide more concise and consistent error messages:
- "Hotkey is invalid" is more direct than "Invalid plugin hotkey"
- New validation messages for empty hotkeys and invalid shortcuts support enhanced validation
These changes align well with the improved validation logic implemented elsewhere in the PR.
Also applies to: 439-439, 448-448
Flow.Launcher.Infrastructure/UserSettings/PluginHotkey.cs (3)
1-1: LGTM! Necessary namespace added.The
Systemnamespace is required for theHashCode.Combinemethod used in theGetHashCodeoverride.
11-15: LGTM! Constructor properly implemented.The constructor provides a convenient way to create instances with both required properties initialized, supporting the refactored dialog workflow.
17-30: LGTM! Value equality correctly implemented.The
EqualsandGetHashCodeoverrides follow .NET best practices:
- Proper type checking with pattern matching
- Comparison of both key properties (
HotkeyandActionKeyword)- Use of
HashCode.Combinefor proper hash code generationThis enables the class to work correctly in collections and supports the validation logic in the view model.
Flow.Launcher/SettingPages/ViewModels/SettingsPaneHotkeyViewModel.cs (3)
72-86: Excellent validation and explicit hotkey management.The enhanced
CustomHotkeyEditmethod introduces important improvements:
- Validates hotkey existence before editing to prevent stale reference issues
- Creates new instances instead of modifying existing ones (immutable approach)
- Explicitly manages hotkey registration/removal in the HotKeyMapper
- Leverages the new equality implementation in
CustomPluginHotkeyThis approach is more robust and prevents potential synchronization issues between the UI and the hotkey system.
92-98: LGTM! Simplified and improved hotkey addition.The refactored
CustomHotkeyAddmethod is cleaner and more explicit:
- Single dialog call with conditional handling
- Creates new hotkey instance using the constructor
- Explicitly registers the hotkey with HotKeyMapper
This centralizes hotkey management in the view model rather than the dialog, which is a better architectural approach.
135-146: LGTM! Consistent validation pattern for shortcuts.The
CustomShortcutEditmethod now follows the same validation pattern as hotkeys:
- Validates shortcut existence before editing
- Uses the found item for replacement to ensure accuracy
- Provides appropriate error feedback using the new localization strings
This creates consistency across the hotkey and shortcut management workflows.
Flow.Launcher/CustomQueryHotkeySetting.xaml.cs (4)
1-11: Good architectural improvement with the new public properties.The refactoring to expose
HotkeyandActionKeywordas public properties aligns well with the separation of concerns principle, allowing the dialog to focus solely on data collection rather than settings management.
32-36: LGTM - Cancel handler is correctly implemented.The explicit setting of
DialogResult = falsebefore closing ensures the calling code can properly detect cancellation.
38-50: The validation and dialog result logic is correct but complex.The validation logic correctly allows either hotkey or action keyword to be set (or both), and the dialog result logic properly handles both add and update scenarios by detecting changes.
The DialogResult logic
!update || originalCustomHotkey.Hotkey != Hotkey || originalCustomHotkey.ActionKeyword != ActionKeywordcorrectly:
- Returns
truefor new hotkeys (add mode)- Returns
truefor updates with changes- Returns
falsefor updates with no changesThis allows the calling view model to determine if settings need to be updated.
59-63: LGTM - Escape handler is consistent with cancel behavior.The escape key handler correctly mirrors the cancel button behavior by setting
DialogResult = false.
|
These two spelling warning cannot be fixed since they are key of the string resources. Other spelling warnings are all handled.
|
jjw24
added
bug
…refactor Refactor CustomQueryHotkeySetting control
…otkeySetting_refactor Refactor CustomQueryHotkeySetting control
Refactor CustomQueryHotkeySetting control to style as CustomShortcutSetting
Seperate from #3770. This refactor is helpful for hotkey model change in #3770.
Test